Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sensors: move baro aggregation to new sensors/vehicle_air_data #14096

Merged
merged 1 commit into from Mar 12, 2020

Conversation

dagar
Copy link
Member

@dagar dagar commented Feb 3, 2020

Continuing to carve up the long running sensors task into smaller WorkItems. This PR collects the barometer voted sensors update and handles it in a new WorkItem (managed within sensors for simplicity) that's synchronized with the primary barometer.

The vehicle_air_data message has been updated with the device_id of the current primary and a timestamp_sample field has been added to get away from the overloaded use.

@dagar dagar force-pushed the pr-vehicle_air_data branch 4 times, most recently from 6746e87 to e16d3c8 Compare February 3, 2020 19:06
@dagar
Copy link
Member Author

dagar commented Feb 3, 2020

I'll need to free up some more flash before we can consider bringing this in.

@dagar dagar changed the title [WIP] sensors: move baro aggregation to new sensors/vehicle_air_data sensors: move baro aggregation to new sensors/vehicle_air_data Feb 4, 2020
@PX4 PX4 deleted a comment from codecov bot Feb 4, 2020
@PX4 PX4 deleted a comment from codecov bot Feb 4, 2020
@dagar dagar marked this pull request as ready for review February 4, 2020 15:55
@dagar dagar requested a review from a team February 4, 2020 17:55
@Tony3dr Tony3dr added this to Ready for testing in Test Flights Feb 4, 2020
@jorge789
Copy link

jorge789 commented Feb 7, 2020

Tested on PixRacer V4

Indoor Flight Test
Modes Tested
Stabilized Mode: Good.

Procedure
Arm and Take off in stabilized mode.

Notes
No issues noted, good flight in general.

Log https://review.px4.io/plot_app?log=16564b9b-1bea-41a3-9f5b-411f7a21d17b

Tested on CUAV nano V5

Indoor Flight Test
Modes Tested
Stabilized Mode: Good.

Procedure
Arm and Take off in stabilized mode.

Notes
No issues noted, good flight in general.

Log https://review.px4.io/plot_app?log=bb4a6782-2699-4f7e-a604-8e03fb0d6727

@dannyfpv
Copy link

dannyfpv commented Feb 7, 2020

Tested on Pixhawk4 v5 f-450
Indoor Flight Test
Modes Tested
Stabilized Mode: Good.

Procedure
Arm and Take off in stabilized mode.

Notes
No issues noted, good flight in general.

Log
https://review.px4.io/plot_app?log=12493ac5-0eea-47f2-b0df-26af528481cd

@Junkim3DR
Copy link

Tested on NXP FMUK66 v3

Indoor Flight Test

@PX4 PX4 deleted a comment from codecov bot Feb 9, 2020
@PX4 PX4 deleted a comment from codecov bot Feb 9, 2020
@dagar
Copy link
Member Author

dagar commented Feb 9, 2020

Hitting the fmu-v2 flash limit again.

@dagar dagar changed the title sensors: move baro aggregation to new sensors/vehicle_air_data [WIP] sensors: move baro aggregation to new sensors/vehicle_air_data Feb 9, 2020
@dagar dagar requested a review from julianoes March 10, 2020 18:09
@dagar dagar changed the title [WIP] sensors: move baro aggregation to new sensors/vehicle_air_data sensors: move baro aggregation to new sensors/vehicle_air_data Mar 10, 2020
@dagar dagar mentioned this pull request Mar 10, 2020
Copy link
Contributor

@julianoes julianoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, see inline comments!

* @category system
* @group Sensor Calibration
*/
PARAM_DEFINE_INT32(CAL_BARO_PRIME, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this was never used, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. We can resurrect easily enough someday if needed.

}

// reschedule timeout
ScheduleDelayed(100_ms);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't you move perf_end above the scheduling?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends what we're trying to measure, but at the moment it's just a rough metric to get an idea of how much time we spend in each WorkItem (relative comparisons, changes over time, etc). The ScheduleDelayed() call is simply adding an entry in a list.

if (uorb_index > 0) {
/* the first always exists, but for each further sensor, add a new validator */
if (!_voter.add_new_validator()) {
PX4_ERR("failed to add validator for sensor_baro:%i", uorb_index);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if this failed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushing in new data later would fail silently. In general we don't handle OOM gracefully all over the system, and I'm not sure if we ever will.

For the data validator in particular it probably makes sense to move this back PX4/Firmware side and not have most of the dynamic allocations in the first place. I'd like to get to the point where we effectively don't have any memory allocations past initial startup.

}

// check for the current best sensor
int best_index = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you initialize it at -1 or is 0 safe?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zero is safe (defaulting to the first baro instance).

@dagar
Copy link
Member Author

dagar commented Mar 11, 2020

Merge conflict with #14344 resolved.

@dagar dagar added this to the Release v1.11.0 milestone Mar 11, 2020
@dagar
Copy link
Member Author

dagar commented Mar 11, 2020

@PX4/testflights could you please do another round of flight testing here? No special requirements.

@Junkim3DR
Copy link

Tested on NXP FMUK66 v3

Indoor Flight Test

@jorge789
Copy link

Tested on PixRacer V4

Indoor Flight Test
Modes Tested
Stabilized Mode: Good.

Procedure
Arm and Take off in stabilized mode.

Notes
No issues noted, good flight in general.

Log https://review.px4.io/plot_app?log=28325e14-8eaf-4a8b-83e2-d522f817e52e

Tested on CUAV nano V5

Indoor Flight Test
Modes Tested
Stabilized Mode: Good.

Procedure
Arm and Take off in stabilized mode.

Notes
No issues noted, good flight in general.

Log https://review.px4.io/plot_app?log=0ce04d81-0146-4c55-9f6f-ee9c260fc944

@dagar dagar merged commit 0eca583 into master Mar 12, 2020
Test Flights automation moved this from Ready for testing to Done Mar 12, 2020
@dagar dagar deleted the pr-vehicle_air_data branch March 15, 2020 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Test Flights
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants